Repository navigation
feat(observability): suppress redundant gRPC auto-instrumentation spans - #18592
chalmerlowe wants to merge 26 commits into
Conversation
…error recording, and templates - Hoist sync channel interceptors (apply_channel_interceptors) and async channel interceptors (apply_async_channel_interceptors) to _compat with active fallback wrapping, eliminating the noop lambda fallback in grpc.py.j2 - Hoist wrap_method tracing and kind support constants to _compat and eliminate inspect.signature module-load checks in base transports - Adopt hybrid error recording in _TraceContext.record_error to enrich span attributes (error.type, status.message) while allowing upstream OpenTelemetry defaults to handle exception events - Pass parameterized proto route template from http_options into trace_http_request url_template - Add rest_transport.kind assertion for AsyncResumableUploadServiceRestTransport to maintain 100% test coverage - Add unit test isolation for custom tracer provider and test cases for _compat interceptors
When OpenTelemetry gRPC auto-instrumentation (GrpcInstrumentorClient().instrument()) is enabled globally alongside Google Cloud SDK tracing, downstream channel calls emit redundant, un-enriched gRPC wire spans. Introduce targeted downstream suppression interceptors for both sync (grpc) and async (grpc.aio) channels using opentelemetry.instrumentation.utils.suppress_instrumentation. This ensures our feature-rich Google Cloud SDK T4 span is emitted while redundant generic auto-instrumentation spans are suppressed.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces client interceptors (_SuppressingClientInterceptor and _AsyncSuppressingClientInterceptor) to suppress redundant downstream generic auto-instrumentation spans when OpenTelemetry is enabled, ensuring that only the feature-rich Google Cloud SDK spans are emitted. It also updates the interceptor pipeline logic to prepend OpenTelemetry interceptors in order. The review feedback suggests critical performance optimizations, specifically moving imports out of the hot path in _suppress_instrumentation to avoid overhead on every RPC call, and removing a redundant inline import of grpc in otel_interceptor.
…typo Address review feedback from Gemini Code Assist and Code Review Council: - Move contextlib and _suppress_instrumentation to module level to eliminate per-RPC dynamic import overhead and ImportError catching in the hot path. - Remove redundant inline import of grpc inside otel_interceptor, safely catching (NameError, AttributeError). - Fix syntax error typo (try:x -> try:) in test_tracing.py.
Replace tuple unpacking (*_GRPC_INTERCEPTOR_BASE) in client interceptor class definitions with explicit base classes and distinct fallback dummy classes when grpc is not installed. Resolves mypy 'Invalid base class [misc]' errors.
…ate test fixtures - Flatten interceptor dispatch in apply_channel_interceptors using upfront target list - Bind grpc.intercept_channel directly to channel and set grpc=None fallback - Add mock_otel_grpc fixture in conftest.py and reuse across test_observability.py - Parametrize sync and async suppression invocation tests across all RPC forms - Parametrize negative interceptor getter tests for disabled and missing OTel
…on tests - Add # type: ignore[misc] with explanatory comment on fallback trace_http_request in _compat.py.j2 - Regenerate Bazel integration goldens with updated _compat.py typing pragma - Gate test_auto_instrumentation_suppression_sync and async with hasattr(_observability, '_AsyncSuppressingClientInterceptor') to prevent failure against released PyPI core
…el fallback - Mark defensive except (AttributeError, TypeError) block in get_otel_interceptor with # pragma: NO COVER - Add unit test verifying graceful fallback when grpc.intercept_channel raises AttributeError or TypeError
| assert child_span.attributes.get("server.address") == "localhost" | ||
| assert child_span.attributes.get("server.port") == 7469 | ||
| assert child_span.attributes.get("url.domain") == "googleapis.com" | ||
| assert child_span.attributes.get("rpc.response.status_code") == "OK" |
There was a problem hiding this comment.
A good deal of the above is duplicative with test_auto_instrumentation_suppression_async()
It also happens to be duplicative with a number of the other existing tests.
I am putting together a PR with two standardized assertion functions to help reduce the duplication across the broader array of tests, but did not want to clutter up this PR.
daniel-sanche
left a comment
daniel-sanche
left a comment
There was a problem hiding this comment.
I don't have time to do a full in-depth review today, but I'd really suggest taking a step back and seeing if there's another way to solve this. This would add a lot of bloat, with the three layers of wrapers fighting each other, which I'm sure would show up in benchmarks. And the workarounds added to apply_channel_interceptors to identify and re-order interceptors feel like a big abstraction overreach
I added a comment about a possible approach to strip out unwanted interceptors before applying our custom otel ones. If that doesn't work, maybe spend some time considering other options, before committing to these new wrapper layers
| channel = grpc.intercept_channel(channel, suppressor) | ||
| except (AttributeError, TypeError): # pragma: NO COVER | ||
| # Fallback if grpc lacks intercept_channel or custom channel type rejects it. | ||
| pass |
There was a problem hiding this comment.
My unerstanding of this fix is that channel may already be wrapped with an interceptor, and we need to add another wrapper to supress it
Instead, can we unwrap the channel here?
def otel_interceptor(channel: grpc.Channel) -> grpc.Channel:
# Peel off any generic upstream auto-instrumentation wrapper once at setup time.
while (
isinstance(getattr(channel, "_interceptor", None), OpenTelemetryClientInterceptor)
and not getattr(channel._interceptor, "_is_otel_interceptor", False)
and hasattr(channel, "_channel")
):
channel = channel._channel
return otel_grpc.intercept_channel(channel, interceptor)
(We could do something similar on the async side, pruning existing the channel's interceptor list before adding our own otel one)
There was a problem hiding this comment.
RECOMMENDATION: LEAVE AS IS
We investigated unwrapping/peeling the channel at setup time as you suggested, but found two significant risks that make runtime suppression the safer path:
-
Dropping Intentionally Configured User Interceptors: Upstream
opentelemetry-instrumentation-grpcprovides a public factory functionclient_interceptor(request_hook=..., filter_=...)that developers use to manually configure custom request hooks or filters without subclassing. The returned object is a direct instance ofOpenTelemetryClientInterceptor. If a user configures a channel with custom hooks and passes it into our client, peeling offOpenTelemetryClientInterceptorat setup time will silently delete the customer's custom hooks. In contrast,suppress_instrumentation()is the upstream contract designed to suppress span creation without mutating/deleting parts of the channel structure. -
Shared Channel Breakage: If a customer passes an existing channel and shares it between our SDK and their own internal gRPC services, stripping/mutating interceptors on the channel permanently breaks tracing for their internal services. Runtime suppression avoids this because
suppress_instrumentation()is thread/task-local and only active during the execution of OUR SDK's call.
| # OpenTelemetry interceptors and downstream suppressors are inserted at the | ||
| # beginning of the channel's interceptor pipeline in order, so that our | ||
| # Google SDK spans wrap outside and downstream generic auto-instrumentation | ||
| # (e.g. GrpcAioInstrumentorClient) is suppressed. Other interceptors are appended. |
There was a problem hiding this comment.
I would really like to avoid this kind of special-case re-ordering if at all possible. Even using a different method for re-ordering, before calling apply_channel_interceptors if you have to
If you need to add it here, it should be called out in the docstring that otel is treated as a special case
There was a problem hiding this comment.
For clarity, the reordering is required to enable the suppression.
We looked for ways to make it simpler and more robust and to help divide the architecture such that the calling function decides whether to prepend OR append the interceptor.
- In
google-api-core(grpc_helpers_async.py): We stripped all_is_otel_interceptorlogic and index math out ofapply_channel_interceptors. It is now a generic utility that acceptsprepend: bool = False. When prepending, it uses Python slice assignment (target_list[:0] = matching) so the batch is inserted at the front in exact relative order without using stateful index counters (also resolving Tony's sequential ordering comment). - In
gapic-generator(grpc_asyncio.py.j2): The transport caller explicitly declares its ordering intent. Normal user and logging interceptors are applied withprepend=False(appended). Observability interceptors are applied withprepend=True, placing them at the front of the channel's execution pipeline so they wrap outside and suppress ambient auto-instrumentation on pre-existing channels.
This keepsgoogle-api-coreagnostic of OpenTelemetry while giving calling functions control over pipeline positioning.
… clean compat - Add __slots__ = () to _SuppressingClientInterceptor, _AsyncSuppressingClientInterceptor, and fallback dummy classes. - Refactor apply_channel_interceptors in grpc_helpers_async.py with prepend: bool = False, using slice assignment (target_list[:0] = matching) to guarantee order stability and prevent index degradation across multiple calls. - In gapic-generator: explicitly call apply_async_channel_interceptors(..., prepend=True) for OpenTelemetry tracing interceptors in grpc_asyncio.py.j2. - Remove test-only HAS_AUTO_INSTRUMENTATION_SUPPRESSION sentinel from _compat.py.j2 and goldens, detecting capabilities locally in test_tracing.py. - Regenerate Bazel integration goldens across all targets. - Move async suppression invocation tests to test_observability_async.py and test prepend=True batch behavior.
…ne guard - In _compat.py.j2: only pass prepend to grpc_helpers_async.apply_channel_interceptors when True with TypeError fallback for older google-api-core releases. - Add None-check guard before hasattr(target_list, 'append') to satisfy mypy union-attr check. - Regenerate Bazel integration goldens.
…terceptors compat shim
…d remove covered pragma
chalmerlowe
left a comment
chalmerlowe
left a comment
There was a problem hiding this comment.
Some suggested changes.
Thanks for the feedback, @daniel-sanche We recommend moving forward with the approach we recommended in the PR. Additional clarification can be found on specific comments, but broadly:
Beyond that:
|
…spans' into feat/otel-tracing-suppress-auto-spans
This branch has not been deployed
3 participants
Intent & Context
When upstream OpenTelemetry gRPC auto-instrumentation (
opentelemetry.instrumentation.grpc.GrpcInstrumentorClient().instrument()) is enabled globally in a customer application alongside Google Cloud SDK tracing, downstream channel invocations create redundant, un-enriched wire attempt spans.This PR introduces targeted downstream suppression interceptors for both sync (
grpc) and async (grpc.aio) transports:suppress_instrumentationcontext manager.feat/otel-tracing-universal-4path).